-
Notifications
You must be signed in to change notification settings - Fork 895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the short_name scope attribute #2702
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it would make things more streamlined if we first tackle the addition of "scope" as a semantic convention concept separately from the Prometheus use-cases/changes. So, a PR only for adding the scope new yaml/md.
This (or a new PR if you decide to split) probably should fix #2682
a788e5e
to
3c80ea8
Compare
Done |
If the Scope has a different namespace then why are we prefixing the attribute names by I am not sure what is the right approach but it seems like it needs to be one or another and then we need to consistent with that decision, which for now we don't seem to be. |
My thinking was that some scope attributes may only apply to a particular definition of scope, such as |
It does look a bit weird. Virtually every other attribute we have in semantic conventions for Resources or Spans or Metrics has some sort of a prefix. We have never defined any attributes in the root namespace although I don't think the rules explicitly prohibit it. However using a prefix On the other hand the benefit of using Sorry, I am probably not helping much. I probably need to make up my mind first :-) |
5f7c784
to
4ee1ff5
Compare
I find strange that we have
It seems Then, other scope attributes don't need any prefix, and can exist by themselves, we just happen to "list" them under the scope semantic conventions bcs it's where they should be applied. And this ties nicely with the next thing:
I would think it's better to not have a mixed approach when it comes to solving the question of how we "flatten" attributes. Like, either all signals have their attributes prefixed or none have. Having a prefix for |
One topic that came up during today's spec call is the idea of making this attribute's behavior consistent across exporters. For example, prometheus adds it as a prefix to metrics, but why shouldn't other exporters do the same? Perhaps other non-OTLP exporters should also add this as a prefix. Maybe something like |
d639496
to
5b37f17
Compare
specification/scope/README.md
Outdated
<!-- semconv scope --> | ||
| Attribute | Type | Description | Examples | Requirement Level | | ||
|---|---|---|---|---| | ||
| `scope.short_name` | string | The short name for the instrumentation scope, which should generally be less than 15 characters, and generally consist of a single word. It is not required to be globally unique, but should be unique enough to make it very unlikely to collide with other short names. An example use is as the [namespace](https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#metric-naming-and-namespaces) (prefix) for OpenMetrics metric names. | `mylibrary`; `otelhttp` | Recommended | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocking comment, but it seems likely that scope.short_name
will make its way into scope attributes for traces and logs as well. I can't think of any reason why this would be a problem - just want to confirm that everyone understands scope.short_name
to be a signal agnostic attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this attribute "special" in some way, compared with others? In the companion #2703 an example is given with two scope attributes, scope.short_name
and library_mascot
.
If I receive an OTLP export (for any signal) with two scopes that contain otherwise identically-named logs/spans/metrics, then:
- the scope.short_name is implicit qualifier for the named logs/spans/metrics, making them distinct and unrelated; metrics belonging to the two scopes cannot internally break the single-writer rule (despite sharing same attributes).
- the library_mascot is just like a resource attribute, has no special properties; metrics belonging to the two scopes would break the single-writer rule, if they were to share the same attributes.
Not sure if my interpretation is correct or not, would like this section to clarify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just like a resource attribute, has no special properties; metrics belonging to the two scopes would break the single-writer rule, if they were to share the same attributes.
This doesn't seem correct for resource attributes. Reporting CPU usage (assume the metric has no attributes) is possible for two different resources (e.g. two different containers) without breaking the single-writer rule, right?
My take is that:
- Fundamentally, the scope (full) name+ version is the implicit qualifier which implies that metrics within it distinct and unrelated. I believe this is currently the status quo, but this may be the question you are asking about.
- All scope attributes are descriptive (not identifying) of the scope name + version
- short_name is a shortened identifier which is likely, but not guaranteed, to uniquely identify the scope name + version within a program.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thank you. I prefer your interpretation.
In data-model.md, there's a paragraph that can use a slight extension:
Metric streams are grouped into individual `Metric` objects,
identified by:
- The originating `Resource` attributes
- The instrumentation `Scope` (e.g., instrumentation library name, version)
- The metric stream's `name`
Maybe change "(e.g., instrumentation library name, version)" to "(i.e., instrumentation scope name, version, and attributes)".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually thinking that scope attributes would be non-identifying: "All scope attributes are descriptive (not identifying) of the scope name + version", as that would reduce the burden on non-otel exporters to REQUIRE them to be attached to all telemetry. Are there use-cases where we want attributes to be identifying?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to have them, since we anyway don't use them to construct different meters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to have them, since we anyway don't use them to construct different meters.
I expect varying scope attributes will produce different meters, seems to be the intention behind #2579, specifically https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#get-a-meter
specifically "[since 1.13.0] attributes (optional): Specifies the instrumentation scope attributes to associate with emitted telemetry." and "Implementations MUST return different Meter instances when called repeatedly with different values of parameters."
I've opted to update the PR to use the prefix-less |
Regarding using So, at the very least, IMO, we cannot have a hard rule that says |
@dashpole does the tooling works in this case, to auto generate the table? (without a prefix) |
I had to add one commit to the scope build-tools PR to allow an empty prefix, but otherwise it works. |
This unblocks #2702 by making the changes introduced in open-telemetry/build-tools#114 available for use. It also allows for more semantic conventions for scope attributes to be defined in the future (#2682). See https://github.com/open-telemetry/build-tools/releases/tag/v0.14.0.
7808057
to
61038e4
Compare
740c4a0
to
b42d588
Compare
ee634c0
to
e45d687
Compare
As discussed in the spec SIG today I wonder whether it would be beneficial to define a requirement that scope attributes be prefixed with |
This results in a desirable outcome for |
Is it to bad to suggest that only root-level attributes get a suffix ( |
Approving, but as mentioned in the Spec call too, let's allow some days for some days to iron out final details. |
(I could not attend the Spec SIG yesterday and the recording is not yet available.) From the discussion above, looks like we're still debating what to call this attribute and whether it should have "otel.scope" as a prefix. @dashpole asked two weeks ago:
I suggest that I think |
I don't think we should flatten for Prometheus/OpenMetrics. I've proposed a join-able info metric in #2703. I think the guidance is for other non-OTLP exporters. A scope is already somewhat of a namespace, and i'm hoping to introduce something thats closely tied to the scope name to avoid creating a new concept. I also worry an un-prefixed namespace would collide with metric/span/logRecord attributes often. |
Is this Scope |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Based on #2703 (comment), this isn't required anymore. |
This unblocks open-telemetry#2702 by making the changes introduced in open-telemetry/build-tools#114 available for use. It also allows for more semantic conventions for scope attributes to be defined in the future (open-telemetry#2682). See https://github.com/open-telemetry/build-tools/releases/tag/v0.14.0.
Fixes #2682
Changes
Add the
short_name
scope attribute. Alternatively, it could have been added asotel.short_name
to distinguish it from non-otel attributes, orprometheus.namespace
to make it explicitly associated with prefixing prometheus metrics.@jmacd @tigrannajaryan @jsuereth